Skip to content

Skip PreventBrowserAccess middleware when running tests #551

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

gwleuverink
Copy link
Contributor

The PreventBrowserAccess middleware causes browser tests to fail with 403's. This PR adds a simple guard clause that skips the middleware when it detects it's running tests.

@gwleuverink gwleuverink changed the title skip PreventBrowserAccess middleware when running tests Skip PreventBrowserAccess middleware when running tests Apr 14, 2025
@gwleuverink gwleuverink self-assigned this Apr 14, 2025
@gwleuverink gwleuverink requested a review from a team April 14, 2025 09:25
@gwleuverink gwleuverink added the enhancement New feature or request label Apr 14, 2025
@gwleuverink gwleuverink marked this pull request as draft April 14, 2025 10:09
@gwleuverink
Copy link
Contributor Author

Converted to Draft. Will address the feedback given on Discord first 👍🏻

@gwleuverink
Copy link
Contributor Author

Couple notes @NativePHP/contributors:

I've added the automatic registration inside a if (config('nativephp-internal.running')) conditional.
Additionally I've also added a guard clause in the middleware itself, in case anyone needs to apply it on other groups.

  • Would it make sense to add it to api group too?
  • We need to update the docs to reflect these changes

@gwleuverink gwleuverink marked this pull request as ready for review April 14, 2025 22:52
@simonhamp
Copy link
Member

@gwleuverink i think it does need to be applied globally, not just to the web group.

I'm thinking of scenarios where an attacker could use the API endpoints to trigger app behaviour in an unwanted manner simply by using curl against the right port...

So, if possible let's go for applying globally.

But we need to make sure that all requests from Electron to the Laravel app include the necessary header...

@gwleuverink
Copy link
Contributor Author

I had to import Foundation\Http\Kernel instead of Contracts\Http\Kernel because phpstan complained the pushMiddleware method is not present on the interface.

@gwleuverink
Copy link
Contributor Author

I tried this in a simple app. Seems to work as expected. Verification welcome!

@simonhamp About the necessary header. Any scenario's this isn't accounted for already?

@simonhamp
Copy link
Member

I don't think so. Just something we need to remember to do in case we add new ways to call the Laravel backend.

@gwleuverink
Copy link
Contributor Author

Shall I remove this section from the docs entirely or leave a note about this behaviour?

At the bottom:
https://nativephp.com/docs/desktop/1/digging-deeper/security#the-web-servers

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants